Cleanup of CI run_tests and run_tests_windows. #1970
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1970 +/- ##
==========================================
- Coverage 83.51% 83.50% -0.01%
==========================================
Files 311 311
Lines 54897 54894 -3
Branches 12213 11897 -316
==========================================
- Hits 45845 45839 -6
- Misses 7788 7835 +47
+ Partials 1264 1220 -44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| pass | ||
| return None, None | ||
|
|
||
| def run_packet_coverage(): |
There was a problem hiding this comment.
This is a method of just one action, why should it be its own method?
There was a problem hiding this comment.
IMO, It's easier to read at the call site. The main function is already quite complex, and it is easier to understand what run_packet_***() does at a glance than having the raw subprocess run command with all the arguments and having to understand what it does.
There was a problem hiding this comment.
I don't think the functions are that complex, moving back and forth between functions is also not very easy to read
There was a problem hiding this comment.
I can revert it, but the way I see it the main function's focus is orchestration and the run_*** functions are the actual implementations of how the actual actions are achieved. The orchestration shouldn't need to worry about how the actions are done, and the actions shouldn't need to be concerned with the orchestration.
It also mirrors the way it is written in run_tests.py, and having the two scripts somewhat similar would be nice.
PS: The tcp replay worker start / stop should probably be moved under their respective run_pcap_*** operations too.
| ) | ||
|
|
||
|
|
||
| def run_pcap_tests(ip_address: str, skip_tests: list[str]): |
There was a problem hiding this comment.
I think that having this class instead of a simple @contextmanager and subprocess.run() to get the nics is an overkill. Usually less code is better 🙂
There was a problem hiding this comment.
It's not just the nics. Those were mostly an afterthought. It's so the tcp replay executable passed around as a structured object. IMO, that is better than having a path in the parameters that you need to know is the executable.
The differences between unix and windows calls are handled internally instead of having to do that all over the place or having duplicated procedures.
It also allows the places where it is used not to worry about how exactly the replay executable is found.
There was a problem hiding this comment.
It's not just the nics. Those were mostly an afterthought. It's so the tcp replay executable passed around as a structured object. IMO, that is better than having a path in the parameters that you need to know is the executable.
The only parameter that is passed around is the tcpreplay path, but I think it's ok because it's just a string.
The differences between unix and windows calls are handled internally instead of having to do that all over the place or having duplicated procedures.
I actually think this is the main problem with the new class - methods like get_nic_list() only run on Windows and otherwise throw an exception, which is not great from OOP perspective. The constructor also has a few if..else per platform - again, not great OOP. I don't think this new class is really needed
There was a problem hiding this comment.
The only parameter that is passed around is the tcpreplay path, but I think it's ok because it's just a string.
Eh, its workable, but IMO a lot less explicit. A string also won't be caught by the type system if it is passed in the wrong place.
I actually think this is the main problem with the new class - methods like get_nic_list() only run on Windows and otherwise throw an exception, which is not great from OOP perspective.
There is nothing preventing the method get_nic_list() to work on unix, as far as I know. It is just that the method was not used in the unix part so I didn't bother. I can add it if it is an issue.
The constructor also has a few if..else per platform - again, not great OOP.
Why is per platform, if else not great OOP exactly?
The only thing that is "per platform" per se, is that on windows:
- the executable path is formatted correctly with an
.exesuffix. - the kill process command uses the
taskkill.
Those are both basic cross platform patches that don't modify anything else in the functionality of the class, and would need to be done regardless if the code is structured as tcp_replay_worker function or as the TcpReplay object since the user only submits an optional directory, if the function is to be cross platform.
There was a problem hiding this comment.
Eh, its workable, but IMO a lot less explicit. A string also won't be caught by the type system if it is passed in the wrong place.
A path is a string... we can convert the string to Python's Path class but I don't think it's necessary here
There is nothing preventing the method
get_nic_list()to work on unix, as far as I know. It is just that the method was not used in the unix part so I didn't bother. I can add it if it is an issue.
To be honest, I didn't check if it can run on Linux, I assume it does and in that case the if sys.platform != "win32" is not needed
Why is per platform,
if elsenot great OOP exactly?
In "clean OOP" there should be minimum if..else clauses and you'd expect to have separate classes for different platforms that share a base class with the common functionality. However, as I mentioned before, I don't think all of this is needed here
There was a problem hiding this comment.
To be honest, I didn't check if it can run on Linux, I assume it does and in that case the if sys.platform != "win32" is not needed
The call to tcpreplay --listnics works but the format is different.
It returns
tcpreplay --listnics
Warning in interface.c:get_interface_list() line 80:
May need to run as root to get access to all network interfaces.
Warning in interface.c:get_interface_list() line 80:
May need to run as root to get access to all network interfaces.
Available network interfaces:
eth0
any
bluetooth-monitor
nflog
nfqueue
dbus-system
dbus-session
instead of \\Device\\NPF_*** strings, so the code after the call is win32 only. That is why the platform is restricted atm.
In "clean OOP" there should be minimum if..else clauses and you'd expect to have separate classes for different platforms that share a base class with the common functionality. However, as I mentioned before, I don't think all of this is needed here
Fair enough. I also agree that full derived classes aren't needed here.
Having said that, we can't escape the if sys.platform branches, if we want to keep the code duplication low and routines cross platform. It is either that or have full duplicated routines for a minor difference. We need to have the enforcement / compatibility checks somewhere.
I used an object so the code is mostly at one place and to maintain the class invariants. It allows the checks / compatibility layer to be centralized. Otherwise it would require every routine that is cross-platform to do the checks in the __init__() method and the checks in the _kill_process() method. Currently that would only be replay / tcp_replay_worker but future maintainability suffers if more functions need to be added.
There was a problem hiding this comment.
What I'm saying is that the current script isn't too complex to read or maintain, which is why I think this refactoring is not really needed. I'd think we can focus our energy in areas that can be more beneficial for the project and its users 🙂
| ) | ||
|
|
||
|
|
||
| def run_pcap_coverage(ip_address: str, skip_tests: list[str]): |
There was a problem hiding this comment.
ditto: This is a method of just one action, why should it be its own method?
The PR cleans up the CI scripts that run the tests.
TcpReplayclass to abstract tcpreplay executable and operations on it.tcp_replay_worker. Uses have been replaced byTcpReplay.replay(...)